-
Notifications
You must be signed in to change notification settings - Fork 10.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add an overload of append(contentsOf:) on Array that takes a Collection instead of a Sequence, and try using it to accelerate wCSIA-compatible Sequences #77487
base: main
Are you sure you want to change the base?
Conversation
…on instead of a Sequence, and try using it to accelerate wCSIA-compatible Sequences
@swift-ci test |
@swift-ci Apple Silicon benchmark |
…lection instead of a Sequence, and try using it to accelerate wCSIA-compatible Sequences
@swift-ci Apple Silicon benchmark |
…a Collection instead of a Sequence, and try using it to accelerate wCSIA-compatible Sequences
@swift-ci Apple Silicon benchmark |
@swift-ci please benchmark |
|
@swift-ci test |
@swift-ci test |
Asked Erik to take a look at the test failure, since it's about the _effects thing we discussed |
stdlib/public/core/Array.swift
Outdated
/// - Complexity: O(*m*) on average, where *m* is the length of | ||
/// `newElements`, over many calls to `append(contentsOf:)` on the same | ||
/// array. | ||
@inlinable @_alwaysEmitIntoClient |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: This is a little misleading: @_alwaysEmitIntoClient
already implies inlinability, but @inlinable
carries an expectation that it will export an ABI symbol. I recommend just using aeic, like we do in most similar cases in the stdlib.
@inlinable @_alwaysEmitIntoClient | |
@_alwaysEmitIntoClient |
Update: D'oh, we appear to be less consistent about this than I thought. The Array
code is pretty consistent though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll fix this once I figure out the SIL test failure :) I had just @inlinable originally but Steve correctly pointed out that would make the Sequence-taking one explode when back-deployed if it didn't get inlined.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
Co-authored-by: Karoy Lorentey <[email protected]>
…ppend-contentsof' into what-about-second-append-contentsof
Not clear to me why pushing that auto-requested review from Erik… |
@swift-ci test |
@swift-ci test |
@swift-ci Apple Silicon benchmark |
@swift-ci test |
@swift-ci Apple Silicon benchmark |
The issue I noted was addressed, but there are parts I still don't understand.
@swift-ci please Apple Silicon benchmark |
@@ -1063,6 +1063,12 @@ extension Sequence { | |||
internal func _copySequenceToContiguousArray< | |||
S: Sequence | |||
>(_ source: S) -> ContiguousArray<S.Element> { | |||
let contigArray = source.withContiguousStorageIfAvailable { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This part is new btw
Well that was completely ridiculous. Wasted several hours because build-script stopped rebuilding the tests, so I kept getting failures from the old code rather than my fix. |
@swift-ci please test |
@swift-ci Apple Silicon benchmark |
Fixes rdar://139455035